#342 - Implemented Label Value Sanitizer #776

Tigraine · 2022-04-22T13:18:24Z

@brian-brazil as discussed (a few years ago 🤣 ) here #342

This PR adds the ability to all SimpleCollector instances to manipulate labels before they are processed.
This allows users to extend a label to be runtime safe or do manipulation like removing credentials etc..

Example would be:

String clientName = getClientName(); // this label is not null-safe
counter.label(clientName).inc() <-- will throw IllegalArgumentException only during runtime

Usage:

metric = Gauge.build()
            .name("nonulllabels")
            .help("help")
            .labelNames("l")
            .labelValueSanitizer(Gauge.TransformNullLabelsToEmptyString())
            .register(registry);

Instead of having to sanitise all labels before calling the instrumentation code we can make the code safe at setup and not care about downstream mis-use.
Also since this is implemented through an interface additional use cases can be passed by consumers of the library.

Signed-off-by: Daniel Hoelbling-Inzko <[email protected]>

fstab

Thanks a lot for the PR. I like the idea of having a generic way to transform labels. I added a few thoughts as review comments. Let me know what you think.

fstab · 2022-05-02T10:24:20Z

simpleclient/src/main/java/io/prometheus/client/LabelValueSanitizer.java

@@ -0,0 +1,5 @@
+package io.prometheus.client;
+
+public interface LabelValueSanitizer {


As you say this can also be used for other use cases, not just "sanitization". So maybe the interface should have a more generic name covering all use cases, like LabelValueTransformer?

I agree with @fstab ... LabelValueTransformer seems like it could be used more generically.

There may be future use cases/places where the paradigm could be used to transform other (non-label) String values, so I feel it would make more sense to rename the interface to StringTransformer.

fstab · 2022-05-02T10:29:15Z

simpleclient/src/main/java/io/prometheus/client/LabelValueSanitizer.java

+package io.prometheus.client;
+
+public interface LabelValueSanitizer {
+    String[] sanitize(String... labelValue);


I think it would be better to sanitize only a single label value, not the whole array. For example, you could register a sanitizer like this:

metric = Gauge.build() .name("nonulllabels") .help("help") .labelNames("path", "status") .labelValueSanitizer("path", myPathSanitizer) .register(registry);

and then the myPathSanitizer would only be called for the path label value, but not for an array of all label values.

Since labelNames takes a String array, I think it makes sense for the class to manipulate the labels also takes a String array.

Depends on what use case you have in mind.

What I have in mind is that you have a specific label that you want to transform (for example stripping a unique user ID from an HTTP path). For that, it would be good to implement a specific pathTransformer and apply it to the path label. Advantage is: You don't need to magically know at which position in the array the path label is. And if path is passed as a label to multiple different metrics, you can reuse the transformer, even if the path is on different positions for each metric.

The array as parameter only makes sense if you want to do exactly the same transformation for all labels. This would work for replacing null with "null", but for every other use case I think it's better if you know which label you want to transform.

Both might actually be interesting.
Maybe having a Transformer be applied to a specific label or * to apply to all?
I could for example think of a case where you'd want to have your infrastructure automatically prevent users from using excessively long labels (truncating them automatically).

fstab · 2022-05-02T10:34:26Z

simpleclient/src/main/java/io/prometheus/client/SimpleCollector.java

+  /**
+   * Transforms null labels to an empty string to guard against IllegalArgument runtime exceptions
+   */
+  static LabelValueSanitizer TransformNullLabelsToEmptyString() {
+    return new LabelValueSanitizer() {
+      @Override
+      public String[] sanitize(String... labelValue) {
+        for (int i = 0; i < labelValue.length; i++) {
+          if (labelValue[i] == null) {
+            labelValue[i] = "";
+          }
+        }
+        return labelValue;
+      }
+    };
+  }
+


If the sanitizer takes only a single string as argument instead of an array of strings, a good handler for null values would be String::valueOf. As this already exists in the Java runtime, there's no need to provide our own implementation in SimpleCollector.

fstab · 2022-05-02T11:46:38Z

Regarding the specific case of null values: If we revisit this and not throw an IllegalArgumentException, what do you think would be a good default value? The literal string "null" as returned by String::valueOf, or an empty value?

Tigraine · 2022-06-02T09:31:00Z

Regarding the specific case of null values: If we revisit this and not throw an IllegalArgumentException, what do you think would be a good default value? The literal string "null" as returned by String::valueOf, or an empty value?

Thanks for the feedback! I will update the PR to move to a Transformer as it makes a lot of sense.

I was thinking about the appropriate default value a bit and in our case 'NULL' would be a perfectly good default value - but that might not be for everyone as a default which is the main reason I came up with this PR and the concept of Sanitization.

dhoard · 2022-07-01T18:23:57Z

simpleclient/src/main/java/io/prometheus/client/SimpleCollector.java

+  static LabelValueSanitizer NoOpLabelValueSanitizer() {
+    return new LabelValueSanitizer() {
+      @Override
+      public String[] sanitize(String... labelValue) {


Change labelValue to labelValues since it's an array.

dhoard · 2022-07-01T18:24:06Z

simpleclient/src/main/java/io/prometheus/client/SimpleCollector.java

+  static LabelValueSanitizer TransformNullLabelsToEmptyString() {
+    return new LabelValueSanitizer() {
+      @Override
+      public String[] sanitize(String... labelValue) {


Change labelValue to labelValues since it's an array.

prometheus#342 - Implemented Label Value Sanitizer

065f82a

Signed-off-by: Daniel Hoelbling-Inzko <[email protected]>

Tigraine force-pushed the master branch from 2bc9d91 to 065f82a Compare April 22, 2022 13:21

fstab requested changes May 2, 2022

View reviewed changes

dhoard reviewed Jul 1, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#342 - Implemented Label Value Sanitizer #776

#342 - Implemented Label Value Sanitizer #776

Tigraine commented Apr 22, 2022

fstab left a comment

fstab May 2, 2022

dhoard May 19, 2022

dhoard Jul 1, 2022 •

edited

Loading

fstab May 2, 2022 •

edited

Loading

dhoard May 19, 2022

fstab May 19, 2022

Tigraine Jun 2, 2022

fstab May 2, 2022

fstab commented May 2, 2022

Tigraine commented Jun 2, 2022

dhoard Jul 1, 2022

dhoard Jul 1, 2022

		@@ -0,0 +1,5 @@
		package io.prometheus.client;

		public interface LabelValueSanitizer {

#342 - Implemented Label Value Sanitizer #776

Are you sure you want to change the base?

#342 - Implemented Label Value Sanitizer #776

Conversation

Tigraine commented Apr 22, 2022

fstab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhoard Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

fstab May 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fstab commented May 2, 2022

Tigraine commented Jun 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhoard Jul 1, 2022 •

edited

Loading

fstab May 2, 2022 •

edited

Loading